-
-
Notifications
You must be signed in to change notification settings - Fork 306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: improve state serialization #6563
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6563 +/- ##
============================================
+ Coverage 61.49% 61.53% +0.03%
============================================
Files 556 557 +1
Lines 58895 58954 +59
Branches 1856 1859 +3
============================================
+ Hits 36216 36275 +59
Misses 22638 22638
Partials 41 41 |
validator: ValueOfFields<typeof ValidatorType> | ||
): number { | ||
output.set(validator.pubkey, offset); | ||
offset += 48; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we could document those constants? Or maybe assign them to variable so that they are more self-explanatory?
Performance Report✔️ no performance regression detected Full benchmark results
|
} | ||
} | ||
|
||
function writeEpochInf(dataView: DataView, offset: number, value: number): number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/state-transition/test/perf/util/serializeState.test.ts
Outdated
Show resolved
Hide resolved
3ee006b
to
121edfe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
name: "lodestar_cp_state_cache_state_persist_seconds", | ||
help: "Histogram of time to persist state to db", | ||
stateSerializeDuration: register.histogram({ | ||
name: "lodestar_cp_state_cache_state_serialize_seconds", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are few other metrics we could consider renaming, those are still unused (not part of dashboard)
stateReloadValidatorsSszDuration: register.histogram({ |
But rather part of another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes will persist all metrics in a follow-up PR
🎉 This PR is included in v1.18.0 🎉 |
Motivation
State serialization happens once per epoch in n-historical state so its performance is important
Description
value_serializeToBytes
performance, this helps makestate.validators.serialize()
3.4x fasterbefore
after
serializeToBytes()
api of ssz v0.15.1part of #5968